Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/muxing: Fix StreamMuxer::Error to io::Error #2664

Conversation

thomaseizinger
Copy link
Contributor

Description

We are already enforcing that the associated type must convert to
io::Error. We might as well just make all functions return an
io::Error directly.

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@thomaseizinger thomaseizinger changed the title Fix StreamMuxer::Error to io::Error core/muxing: Fix StreamMuxer::Error to io::Error May 23, 2022
@thomaseizinger thomaseizinger force-pushed the refactor/remove-error-assoc-type branch from 799dbdb to a402553 Compare May 23, 2022 08:29
We are already enforcing that the associated type must convert to
`io::Error`. We might as well just make all functions return an
`io::Error` directly.
yamux::ConnectionError::Io(e) => e,
e => io::Error::new(io::ErrorKind::Other, e),
}
fn to_io_error(e: yamux::ConnectionError) -> io::Error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn to_io_error(e: yamux::ConnectionError) -> io::Error {
fn into_io_error(e: yamux::ConnectionError) -> io::Error {

https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thinking! Will include in libp2p/rust-yamux#135.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomaseizinger
Copy link
Contributor Author

Replaced with upstreaming an API change: libp2p/rust-yamux#135

@thomaseizinger
Copy link
Contributor Author

Re-opening because there was a misunderstanding in what will be upstreamed.

@thomaseizinger
Copy link
Contributor Author

I'd like to merge #2703 before I start fixing the conflicts here, also because this depends on libp2p/rust-yamux#136.

@thomaseizinger
Copy link
Contributor Author

I'd like to merge #2703 before I start fixing the conflicts here, also because this depends on libp2p/rust-yamux#136.

Technically it doesn't depend on it actually but it would make sense to only touch this code once and use the upstream From implementation directly.

@mxinden
Copy link
Member

mxinden commented Jun 15, 2022

We are already enforcing that the associated type must convert to io::Error. We might as well just make all functions return an io::Error directly.

Sorry for only commenting now. I am undecided whether I agree with the reasoning. I don't expect many people to implement StreamMuxer, thus I am not sure the convenience is worth the lost type information.

I am guessing the argument here is that the concrete error would not bubble up all the way to the user anyways?

@thomaseizinger
Copy link
Contributor Author

I am guessing the argument here is that the concrete error would not bubble up all the way to the user anyways?

The argument is that we are already forcing the associated Error type to convert into io::Error to be able to construct StreamMuxerBox (which needs a concrete error type). We can keep the associated type but then I would suggest to move the bound from the associated type to the constructor of StreamMuxerBox.

In libp2p-swarm, we are only dealing with StreamMuxerBox and thus always have std::io::Error as the error type.

@elenaf9
Copy link
Contributor

elenaf9 commented Jun 15, 2022

I am guessing the argument here is that the concrete error would not bubble up all the way to the user anyways?

The argument is that we are already forcing the associated Error type to convert into io::Error to be able to construct StreamMuxerBox (which needs a concrete error type). We can keep the associated type but then I would suggest to move the bound from the associated type to the constructor of StreamMuxerBox.

In libp2p-swarm, we are only dealing with StreamMuxerBox and thus always have std::io::Error as the error type.

Third option would be to not have a trait bound, but instead in StreamMuxerBox create a io::ErrorKind::Other for the inner error. That's how it is done in transport::Boxed:

.map_err(|e| e.map(box_err))?;

fn box_err<E: Error + Send + Sync + 'static>(e: E) -> io::Error {
io::Error::new(io::ErrorKind::Other, e)
}

Not saying that this is the best solution (if the inner error also has a io::Error variant it would still wap it in an ErrorKind::Other, which is not ideal), just wanted to throw that option in. If you decide to go for the trait bound I'd propose to change it for transport::Boxed as well.

@thomaseizinger
Copy link
Contributor Author

I am guessing the argument here is that the concrete error would not bubble up all the way to the user anyways?

The argument is that we are already forcing the associated Error type to convert into io::Error to be able to construct StreamMuxerBox (which needs a concrete error type). We can keep the associated type but then I would suggest to move the bound from the associated type to the constructor of StreamMuxerBox.
In libp2p-swarm, we are only dealing with StreamMuxerBox and thus always have std::io::Error as the error type.

Third option would be to not have a trait bound, but instead in StreamMuxerBox create a io::ErrorKind::Other for the inner error. That's how it is done in transport::Boxed:

I think this is actually my favourite out of all the proposed ideas. What do you think @mxinden? This would also improve consistency within the codebase. Thanks for the pointer @elenaf9, I should have really looked at transport::boxed myself :)

@thomaseizinger
Copy link
Contributor Author

What do you think of #2710 as an alternative? This is using the strategy suggested by @elenaf9 and what is used in transport::boxed.

@mxinden
Copy link
Member

mxinden commented Jun 19, 2022

What do you think of #2710 as an alternative? This is using the strategy suggested by @elenaf9 and what is used in transport::boxed.

I like #2710 the best.

We can keep the associated type but then I would suggest to move the bound from the associated type to the constructor of StreamMuxerBox.

Would it be worth including this in #2710 as well? In other words, would it be worth it delegating into-io-error-conversion to the error itself by requiring T::Error: Into<io::Error>, on StreamMuxerBox instead of wrapping each T::Error in an io::Error::new(io::ErrorKind::Other like it is done in into_io_error?

@thomaseizinger
Copy link
Contributor Author

Closing this in favor of #2710.

@thomaseizinger
Copy link
Contributor Author

We can keep the associated type but then I would suggest to move the bound from the associated type to the constructor of StreamMuxerBox.

Would it be worth including this in #2710 as well? In other words, would it be worth it delegating into-io-error-conversion to the error itself by requiring T::Error: Into<io::Error>, on StreamMuxerBox instead of wrapping each T::Error in an io::Error::new(io::ErrorKind::Other like it is done in into_io_error?

I am not a fan. io::Error delegates to the inner error anyway so there isn't any unnecessary wrapping and this way, implementations of StreamMuxer can stay unaware that they will ever be converted into an io::Error.

@mxinden
Copy link
Member

mxinden commented Jun 20, 2022

But what if the inner error is already an io::Error @thomaseizinger?

@thomaseizinger
Copy link
Contributor Author

But what if the inner error is already an io::Error @thomaseizinger?

Then it will effectively behave as that io::Error in regards to Display-impl, source, etc.

@thomaseizinger
Copy link
Contributor Author

@mxinden
Copy link
Member

mxinden commented Jun 21, 2022

👍 Sounds good. Thanks for explaining @thomaseizinger!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants